Skip to content

crypto,quic: add NULL checks for OpenSSL allocation functions#63040

Open
armorbreak001 wants to merge 2 commits into
nodejs:mainfrom
armorbreak001:fix/crypto-null-checks
Open

crypto,quic: add NULL checks for OpenSSL allocation functions#63040
armorbreak001 wants to merge 2 commits into
nodejs:mainfrom
armorbreak001:fix/crypto-null-checks

Conversation

@armorbreak001

@armorbreak001 armorbreak001 commented Apr 29, 2026

Copy link
Copy Markdown

Fixes: #62774

This PR adds graceful NULL checks for OpenSSL allocation function return values, replacing CHECK() assertions that would abort the process on allocation failure.

Changes

src/crypto/crypto_aes.ccAES_Cipher()

Replaced CHECK(ctx) with:

if (!ctx) {
  return WebCryptoCipherStatus::FAILED;
}

This matches the pattern already used in AES_CTR_Cipher2() in the same file.

src/crypto/crypto_cipher.ccCipherBase::CommonInit()

Replaced CHECK(ctx_) with:

if (!ctx_) {
  return ThrowCryptoError(env(),
                          mark_pop_error_on_return.peekError(),
                          "Failed to allocate cipher context");
}

This matches the error handling pattern used elsewhere in the same function.

Note on other locations from #62774

  • AES_CTR_Cipher2() — Already has a proper if (!ctx) check (line 238)
  • TLSSession::Initialize() — Already has a proper if (!ssl) check (line 794)
  • ECKeyExportTraits::DoExport() — This function no longer exists in the current codebase; the EC key handling code has been refactored

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 29, 2026
@armorbreak001 armorbreak001 force-pushed the fix/crypto-null-checks branch from 234ce29 to 78ab09a Compare April 29, 2026 20:59
@armorbreak001 armorbreak001 changed the title crypto: add NULL checks for OpenSSL allocation functions crypto,quic: add NULL checks for OpenSSL allocation functions Apr 29, 2026
@armorbreak001 armorbreak001 force-pushed the fix/crypto-null-checks branch from 78ab09a to cb76fb6 Compare May 16, 2026 01:19
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented May 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.05%. Comparing base (5d578c5) to head (1fb33d5).
⚠️ Report is 233 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_cipher.cc 0.00% 2 Missing and 1 partial ⚠️
src/crypto/crypto_aes.cc 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63040      +/-   ##
==========================================
+ Coverage   89.65%   90.05%   +0.39%     
==========================================
  Files         708      714       +6     
  Lines      220402   225525    +5123     
  Branches    42269    42642     +373     
==========================================
+ Hits       197597   203087    +5490     
+ Misses      14671    14205     -466     
- Partials     8134     8233      +99     
Files with missing lines Coverage Δ
src/crypto/crypto_aes.cc 53.63% <0.00%> (-0.19%) ⬇️
src/crypto/crypto_cipher.cc 77.13% <0.00%> (-0.31%) ⬇️

... and 150 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Replace CHECK() assertions with graceful error handling for
EVP_CIPHER_CTX_new() allocations that could fail under memory
pressure:

- crypto_aes.cc (AES_Cipher): return FAILED status
- crypto_cipher.cc (CommonInit): throw JS error via ThrowCryptoError

Fixes nodejs#62774

Signed-off-by: armorbreak001 <contact@agentvote.cc>
@armorbreak001 armorbreak001 force-pushed the fix/crypto-null-checks branch from cb76fb6 to 3899839 Compare May 16, 2026 07:07
Signed-off-by: armorbreak001 <contact@agentvote.cc>
@armorbreak001

Copy link
Copy Markdown
Author

Hi @nodejs/crypto team — just wanted to follow up on this PR. It adds NULL checks for OpenSSL allocation functions in crypto_aes.cc and crypto_cipher.cc, which could prevent potential crashes on allocation failure.

I understand the team is busy. If there are any changes needed or if this isn't the right approach, I'm happy to adjust. Thanks for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crypto,quic: missing NULL checks for OpenSSL allocation functions

3 participants